It seems the name is lost after searilization. I am not sure whether we need to restore the object from the string

The name are not used for TableReferenceExpression and literal argument name as well. These are not serialized as named argument call. I don't see this is restored from serialized string, looks mainly to convert it sql query looks

Do you think we need add a case to veriy:

1. whether we can store a call with model reference as a view via org.apache.flink.table.api.TableEnvironment#createView(java.lang.String, org.apache.flink.table.api.Table)
2. whether we can restore the view and run tests?

I added a IT test for 1. I'm not sure what you mean for 2. Do you mean table api restore test? I don't see restore test support table api though

We should add a test to QueryOperationSqlSerializationTest.

Can we maintain the stack and do the relBuilder.build() in the QueryOperationConverter?

It's hard to do since inputStack depends on this call. Maybe that's why TableReferenceExpression was handled in QueryOperationConverter in the first place

Can we do like this?

  Collections.reverse(resolvedArgs);
  final List<RelNode> inputStack = new ArrayList<>();
  final List<RexNode> rexNodeArgs =
          resolvedArgs.stream()
                  .map(QueryOperationConverter.this::convertExprToRexNode)
                  .peek(
                          expr -> {
                              if (expr instanceof RexTableArgCall) {
                                  inputStack.add(relBuilder.build());
                              }
                          })
                  .collect(Collectors.toList());
  Collections.reverse(rexNodeArgs);

And in the ExpressionConverter:

 @Override
    public RexNode visit(TableReferenceExpression tableRef) {
        final LogicalType tableArgType = tableRef.getOutputDataType().getLogicalType();
        final RelDataType rowType = typeFactory.buildRelNodeRowType((RowType) tableArgType);
        final int[] partitionKeys;
        if (tableRef.getQueryOperation() instanceof PartitionQueryOperation) {
            final PartitionQueryOperation partitionOperation =
                    (PartitionQueryOperation) tableRef.getQueryOperation();
            partitionKeys = partitionOperation.getPartitionKeys();
        } else {
            partitionKeys = new int[0];
        }
        final RexTableArgCall tableArgCall =
                new RexTableArgCall(rowType, relBuilder.size() - 1, partitionKeys, new int[0]);
        //        inputStack.add(relBuilder.build());
        return tableArgCall;
    }

After the change, I find no test in ProcessTableFunctionSemanticTests fails.

I think the logic for ptf(table t1, table t2, arg) is:

1. When building the FunctionQueryOperation, the planner first pushes
   all operands used in the RelNode tree onto the stack. As a result,
   the stack head is t2, followed by t1.
2. We then parse the operands in reverse order. For operand table t2,
   the current stack size is 2, so we can build a RexTableArgCall to
   refer to the stack head. After visiting, we pop the stack head.

WDYT?

Looks we still need to special handle RexTableArgCall here, how about we leave original logic for TableReferenceExpression and just move ModelReferenceExpression to ExpressionConverter?

okay.. Can you open a ticket to track this problems?

Created https://issues.apache.org/jira/browse/FLINK-38638

Is method is unnecessary. It is like "Descriptor in a descriptor". You can just directly pass ColumnList to any method that takes expressions.

nit: I'm still wondering whether we should call this fromModel. It would align better with fromCall.

Call this fromModel to distinguish between the heavily overloaded from() method. We should let from() be reserved for tables.

we don't need to call getName here, ideally we can just use ApiExpressionUtils.unresolvedCall and operationTreeBuilder and pass the FunctionDefinition. So one level deeper. this avoids a catalog lookup for the function name. catalog lookups are expensive.

TableEnvironment.fromCall has additional checks like

tableReferenceChecker.check(arguments);

which seems useful though

Resolve all imports in this class:

time attributes are not a topic for models

nit: simplify code

Is this test still necessary after all semantic tests?

Addressed one of Shengkai's comment to test createView table api